Bound the size of cache in deprecation logger #16724

andrross · 2024-11-26T21:14:28Z

The current implementation of the map used to de-duplicate deprecation log
messages can grow without bound. This adds a simple fixed limit to the data
structure tracking existing loggers. Once the limit is breached new loggers will
not be deduplicated. I also added a check to skip the tracking if the
deprecation logger is disabled.

Related Issues

Resolves #16702

Check List

Functionality includes testing.
~~API changes companion pull request created, if applicable.~~
~~Public documentation issue/PR created, if applicable.~~

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

dblock

Is not using a concurrent hash map a premature optimization? After all even at high volume of requests you’re not going to hit a lock often (concurrent maps are very fast and use atomic swaps as far as I remember).

server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java

github-actions · 2024-11-26T22:08:16Z

❌ Gradle check result for 1913d2b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

andrross · 2024-11-26T22:10:04Z

Is not using a concurrent hash map a premature optimization?

@dblock I don't think using the OpenSearch Cache is an optimization. It's definitely slower! The code is simple though, which is why I chose it.

What alternative do you have in mind with a concurrent hash map? The choices I see are: (1) stop deduplicating when the map hits a certain capacity, or (2) track insertion order to know which ones to evict. I could maybe be convinced that option 1 is good enough, but option 2 is what I implemented using the existing Cache data structure.

reta · 2024-11-27T06:05:11Z

The choices I see are: (1) stop deduplicating when the map hits a certain capacity,

I would agree with @dblock here, the Cache does much more than (expiration tracking, etc) we need, plus we actually introduce more boxing operations using hash code (int -> Integer). May be ConcurrentHashMap is not a bad idea:

    public boolean isAlreadyLogged() {
        return keys.size() <  MAX_DEDUPE_CACHE_ENTRIES && keys.putIfAbsent(keyWithXOpaqueId, Boolean.TRUE) == null;
    }

andrross · 2024-11-27T20:50:23Z

stop deduplicating when the map hits a certain capacity

@reta Agreed that Cache does a lot more than we need (mostly around stats...time-based expiration is a no-op if you don't configure it), but it does give us the LRU semantics which seem beneficial here. The pattern I worry about if we have a simple capacity limit is if you have clients that periodically rotate their opaque ID, then at some point if you leave an OpenSearch server running long enough then it will just stop de-duplicating deprecation warnings.

Also the hashcode thing was to make the memory utilization deterministic as every entry is the same size (a boxed integer). I'm happy to change this to a limited-capacity concurrent set of strings if you think that is good enough. That solution is definitely simple and fast.

server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java

reta · 2024-11-28T06:08:34Z

@reta Agreed that Cache does a lot more than we need (mostly around stats...time-based expiration is a no-op if you don't configure it), but it does give us the LRU semantics which seem beneficial here.

Thanks @andrross , I would have expected us to use LRU semantics but Cache is configured without any expiration policies set (please correct me if I am wrong), so LRU is not used. I also very much aligned with @dblock on the simplicity part, this flow is far from critical and it looks to me that logging the deprecation warning 65k times (at worst, which I believe would very likely be the same deprecated feature triggered over and over again) is sufficient to be noticeable.

server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java

andrross · 2024-11-29T17:53:05Z

I would have expected us to use LRU semantics but Cache is configured without any expiration policies set (please correct me if I am wrong), so LRU is not used

@reta Without expiration policies the Cache will track insertion order, so you're correct it is not LRU, but the basic point is that the Cache will allow new entries to be deduplicated after the limit is reached. However, I can totally buy the argument that this is not a critical code path and it is better to be simple and fast than adding overhead to handle edge cases.

andrross · 2024-11-29T18:44:26Z

@reta I pushed a simple version that simply enforces a size limit in the map. Let me know what you think.

I also removed the hashcode() change, which now means we don't really have control over the amount of memory used as the size of the keys are controlled by the caller. But in practice I think this map would be at under 10MB unless unreasonably large keys are used.

reta · 2024-11-29T19:01:03Z

@dblock I think you would like it ;-)

github-actions · 2024-11-29T19:35:45Z

✅ Gradle check result for 68e9bfc: SUCCESS

codecov · 2024-11-29T19:36:11Z

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 72.07%. Comparing base (d2a1477) to head (6d353b6).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...g/opensearch/common/logging/DeprecatedMessage.java	87.50%	0 Missing and 1 partial ⚠️
...g/opensearch/common/logging/DeprecationLogger.java	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #16724      +/-   ##
============================================
- Coverage     72.16%   72.07%   -0.09%     
+ Complexity    65257    65214      -43     
============================================
  Files          5318     5318              
  Lines        303988   303993       +5     
  Branches      43987    43990       +3     
============================================
- Hits         219358   219113     -245     
- Misses        66674    66934     +260     
+ Partials      17956    17946      -10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java

The current implementation of the map used to de-duplicate deprecation log messages can grow without bound. This adds a simple fixed limit to the data structure tracking existing loggers. Once the limit is breached new loggers will not be deduplicated. I also added a check to skip the tracking if the deprecation logger is disabled. Signed-off-by: Andrew Ross <[email protected]>

github-actions · 2024-12-03T06:17:54Z

❌ Gradle check result for 6d353b6: TIMEOUT

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-12-03T17:32:32Z

✅ Gradle check result for 6d353b6: SUCCESS

dblock

I'm good with this, @reta? @shwetathareja?

opensearch-trigger-bot · 2024-12-03T20:00:35Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-16724-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b1bf72f26e2681e4dbe726bc9605209675f6ab38
# Push it to GitHub
git push --set-upstream origin backport/backport-16724-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-16724-to-2.x.

The current implementation of the map used to de-duplicate deprecation log messages can grow without bound. This adds a simple fixed limit to the data structure tracking existing loggers. Once the limit is breached new loggers will no longer log deprecation warnings. I also added a check to skip the tracking if the deprecation logger is disabled. Signed-off-by: Andrew Ross <[email protected]> (cherry picked from commit b1bf72f)

The current implementation of the map used to de-duplicate deprecation log messages can grow without bound. This adds a simple fixed limit to the data structure tracking existing loggers. Once the limit is breached new loggers will no longer log deprecation warnings. I also added a check to skip the tracking if the deprecation logger is disabled. Signed-off-by: Andrew Ross <[email protected]> (cherry picked from commit b1bf72f) Signed-off-by: Andrew Ross <[email protected]>

The current implementation of the map used to de-duplicate deprecation log messages can grow without bound. This adds a simple fixed limit to the data structure tracking existing loggers. Once the limit is breached new loggers will no longer log deprecation warnings. I also added a check to skip the tracking if the deprecation logger is disabled. (cherry picked from commit b1bf72f) Signed-off-by: Andrew Ross <[email protected]>

andrross requested review from anasalkouz, ashking94, Bukhtawar, CEHENKLE, dblock, dbwiddis, gbbafna, jainankitk, kotwanikunal, linuxpi, mch2, msfroh, nknize, owaiskazi19, reta, Rishikesh1159, sachinpkale, saratvemulapalli, shwetathareja, sohami and VachaShah as code owners November 26, 2024 21:14

github-actions bot added bug Something isn't working Other labels Nov 26, 2024

dblock reviewed Nov 26, 2024

View reviewed changes

sandeshkr419 reviewed Nov 26, 2024

View reviewed changes

server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java Outdated Show resolved Hide resolved

andrross added the backport 2.x Backport to 2.x branch label Nov 26, 2024

opensearch-ci-bot mentioned this pull request Nov 27, 2024

[AUTOCUT] Gradle Check Flaky Test Report for SpecificClusterManagerNodesIT #15944

Open

shwetathareja reviewed Nov 28, 2024

View reviewed changes

server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java Outdated Show resolved Hide resolved

rajiv-kv reviewed Nov 28, 2024

View reviewed changes

server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java Show resolved Hide resolved

andrross force-pushed the deprecation-logger-cache branch from 1913d2b to 68e9bfc Compare November 29, 2024 18:41

reta approved these changes Nov 29, 2024

View reviewed changes

shwetathareja reviewed Dec 2, 2024

View reviewed changes

server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java Outdated Show resolved Hide resolved

shwetathareja reviewed Dec 2, 2024

View reviewed changes

server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java Outdated Show resolved Hide resolved

andrross force-pushed the deprecation-logger-cache branch from 68e9bfc to 7e3567f Compare December 3, 2024 04:15

andrross force-pushed the deprecation-logger-cache branch from 7e3567f to 6d353b6 Compare December 3, 2024 04:15

dblock approved these changes Dec 3, 2024

View reviewed changes

reta approved these changes Dec 3, 2024

View reviewed changes

andrross merged commit b1bf72f into opensearch-project:main Dec 3, 2024
39 checks passed

andrross deleted the deprecation-logger-cache branch December 3, 2024 19:59

opensearch-trigger-bot bot added the backport-failed label Dec 3, 2024

andrross mentioned this pull request Dec 3, 2024

[Backport 2.x] Bound the size of cache in deprecation logger (#16724) #16770

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bound the size of cache in deprecation logger #16724

Bound the size of cache in deprecation logger #16724

andrross commented Nov 26, 2024 •

edited

Loading

dblock left a comment

github-actions bot commented Nov 26, 2024

andrross commented Nov 26, 2024

reta commented Nov 27, 2024 •

edited

Loading

andrross commented Nov 27, 2024 •

edited

Loading

reta commented Nov 28, 2024 •

edited

Loading

andrross commented Nov 29, 2024

andrross commented Nov 29, 2024 •

edited

Loading

reta commented Nov 29, 2024

github-actions bot commented Nov 29, 2024

codecov bot commented Nov 29, 2024 •

edited

Loading

github-actions bot commented Dec 3, 2024

github-actions bot commented Dec 3, 2024

dblock left a comment

opensearch-trigger-bot bot commented Dec 3, 2024

Bound the size of cache in deprecation logger #16724

Bound the size of cache in deprecation logger #16724

Conversation

andrross commented Nov 26, 2024 • edited Loading

Related Issues

Check List

dblock left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 26, 2024

andrross commented Nov 26, 2024

reta commented Nov 27, 2024 • edited Loading

andrross commented Nov 27, 2024 • edited Loading

reta commented Nov 28, 2024 • edited Loading

andrross commented Nov 29, 2024

andrross commented Nov 29, 2024 • edited Loading

reta commented Nov 29, 2024

github-actions bot commented Nov 29, 2024

codecov bot commented Nov 29, 2024 • edited Loading

Codecov Report

github-actions bot commented Dec 3, 2024

github-actions bot commented Dec 3, 2024

dblock left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Dec 3, 2024

andrross commented Nov 26, 2024 •

edited

Loading

reta commented Nov 27, 2024 •

edited

Loading

andrross commented Nov 27, 2024 •

edited

Loading

reta commented Nov 28, 2024 •

edited

Loading

andrross commented Nov 29, 2024 •

edited

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading